Closed
Bug 1229519
Opened 10 years ago
Closed 9 years ago
Toolkit code should pass eslint wherever possible
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla45
| Tracking | Status | |
|---|---|---|
| firefox45 | --- | fixed |
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(9 files, 1 obsolete file)
|
40 bytes,
text/x-review-board-request
|
mak
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
rhelmer
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
mconley
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
mak
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
mak
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
Dolske
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
|
40 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
No description provided.
|
Assignee | |
Comment 1•9 years ago
|
||
| Comment hidden (off-topic) |
|
|
Assignee | |
Comment 3•9 years ago
|
||
Bug 1229519: Fix toolkit/modules to pass eslint checks.
Attachment #8695376 -
Flags: review?(felipc)
|
|
Assignee | |
Comment 4•9 years ago
|
||
Bug 1229519: Fix toolkit/components/thumbnails to pass eslint checks.
Attachment #8695377 -
Flags: review?(rhelmer)
|
|
Assignee | |
Comment 5•9 years ago
|
||
Bug 1229519: Fix toolkit/components/contentprefs to pass eslint checks.
Attachment #8695378 -
Flags: review?(mconley)
|
|
Assignee | |
Comment 6•9 years ago
|
||
Bug 1229519: Fix download managers to pass eslint checks.
Attachment #8695379 -
Flags: review?(gijskruitbosch+bugs)
|
|
Assignee | |
Comment 7•9 years ago
|
||
Bug 1229519: Fix crash manager to pass eslint checks.
Attachment #8695380 -
Flags: review?(mak77)
|
|
Assignee | |
Comment 8•9 years ago
|
||
Bug 1229519: Fix toolkit/components/satchel to pass eslint checks.
Attachment #8695381 -
Flags: review?(dolske)
|
|
Assignee | |
Comment 9•9 years ago
|
||
Bug 1229519: Fix toolkit/components/telemetry to pass eslint checks.
Attachment #8695382 -
Flags: review?(gfritzsche)
|
|
Assignee | |
Comment 10•9 years ago
|
||
Bug 1229519: Fix toolkit/content to pass eslint checks.
Attachment #8695384 -
Flags: review?(felipc)
|
|
Assignee | |
Comment 11•9 years ago
|
||
Bug 1229519: Fix miscellaneous parts of toolkit to pass eslint checks.
Attachment #8695385 -
Flags: review?(MattN+bmo)
|
|
Assignee | |
Comment 12•9 years ago
|
||
Updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c59e1086dbb
|
||
Comment 13•9 years ago
|
||
Comment on attachment 8695382 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/telemetry to pass eslint checks.
https://reviewboard.mozilla.org/r/27095/#review24485
Looks good, thanks!
::: toolkit/components/telemetry/TelemetryStorage.jsm:1237
(Diff revision 1)
> + if (!e.becauseExists)
> + throw e;
Wrap the body in {} please.
::: toolkit/components/telemetry/TelemetryStorage.jsm:1304
(Diff revision 1)
> + if (!(e instanceof OS.File.Error) || !e.becauseNoSuchFile)
> + throw e;
Wrap the body in {} please.
Attachment #8695382 -
Flags: review?(gfritzsche) → review+
|
||
Comment 14•9 years ago
|
||
Comment on attachment 8695378 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/contentprefs to pass eslint checks.
https://reviewboard.mozilla.org/r/27087/#review24501
Attachment #8695378 -
Flags: review?(mconley) → review+
|
||
Comment 15•9 years ago
|
||
Comment on attachment 8695385 [details]
MozReview Request: Bug 1229519: Fix miscellaneous parts of toolkit to pass eslint checks.
https://reviewboard.mozilla.org/r/27099/#review24491
::: .eslintignore:169
(Diff revision 1)
> +# toolkit/exclusions
Nit: "toolkit exclusions"?
::: toolkit/components/microformats/Microformats.js:10
(Diff revision 1)
> /* Custom iterator so that microformats can be enumerated as */
> /* for (i in Microformats) */
> - __iterator__: function () {
> + __iterator__: function* () {
> for (let i=0; i < this.list.length; i++) {
> yield this.list[i];
I don't know if this will work but tests should catch that
::: toolkit/components/social/test/xpcshell/head.js:135
(Diff revision 1)
> // val is an iterator => prepend it to the queue and start on it
> // val is otherwise truthy => call next
> - if (val) {
> + if (value) {
Why change the variable name? The comments are now stale.
::: toolkit/profile/content/createProfileWizard.js:122
(Diff revision 1)
> -#ifndef XP_MACOSX
> + if (AppConstants.platform == "macosx")
> - finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishText");
> + finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishText");
> -#else
> + else
> - finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishTextMac");
> + finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishTextMac");
> -#endif
New code is supposed to brace single-line bodies…
I didn't say anything for the catch blocks since the lines were short but in this case line 123 wrapped in mozreview so it was harder to read without the braces.
::: toolkit/profile/content/profileSelection.js:7
(Diff revision 1)
> Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/AppConstants.jsm");
Nit: I would sort these
::: toolkit/profile/content/profileSelection.js:135
(Diff revision 1)
> switch( aEvent.keyCode )
> {
May as well cleanup the trailing whitespace here as I hope we can have a rule for that in eslint soon.
Attachment #8695385 -
Flags: review?(MattN+bmo) → review+
|
|
||
Comment 16•9 years ago
|
||
https://reviewboard.mozilla.org/r/27087/#review24503
::: toolkit/components/contentprefs/nsContentPrefService.js:1074
(Diff revision 1)
> + if (e.result != Cr.NS_ERROR_FILE_CORRUPTED)
> + throw e;
According to MattN, we're bracing one-liners now.
::: toolkit/components/contentprefs/tests/unit_cps2/AsyncRunner.jsm:49
(Diff revision 1)
> - if (typeof(val) != "boolean")
> - this._iteratorQueue.unshift(val);
> + if (typeof(value) != "boolean")
> + this._iteratorQueue.unshift(value);
Brace the one-liner.
|
|
||
Comment 17•9 years ago
|
||
There was a discussion about this around the time of "goto fail" and it was added to the style guide and I think that's why Georg also mentioned it before me:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
"Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning."
|
|
Assignee | |
Comment 18•9 years ago
|
||
I attempted to make the bracing match the surrounding code but happy to switch it to brace everything. At some point we'll consider turning the curly rule on for browser/toolkit and then there will be a reckoning.
|
||
Comment 19•9 years ago
|
||
Note I have a volunteer working on removing array comprehensions from Places in bug 1228976, so please don't touch that for now
|
|
||
Comment 20•9 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #17)
> "Always brace controlled statements, even a single-line consequent of an if
> else else. This is redundant typically, but it avoids dangling else bugs, so
> it's safer at scale than fine-tuning."
IIRC that was directly imported from the cpp coding style but broad consensus was never reached about single line ifs. Afaict nobody cares about that rule, indeed I keep seeing patches not bracing single line ifs.
It's also unclear (to me) if the coding style means to brace single-line only if they are part of an if-else.
Either we intend the rule as "always brace if part of an if-else(if)" as de-facto we did until today, or at this point we enforce bracing everywhere through eslint.
|
|
||
Updated•9 years ago
|
Attachment #8695380 -
Flags: review?(mak77) → review+
|
|
||
Comment 21•9 years ago
|
||
Comment on attachment 8695380 [details]
MozReview Request: Bug 1229519: Fix crash manager to pass eslint checks.
https://reviewboard.mozilla.org/r/27091/#review24515
::: toolkit/components/crashmonitor/CrashMonitor.jsm:193
(Diff revision 1)
> - Task.spawn(function() {
> + Task.spawn(function*() {
missing space after *
|
||
Updated•9 years ago
|
Attachment #8695381 -
Flags: review?(dolske) → review+
|
|
||
Comment 22•9 years ago
|
||
Comment on attachment 8695381 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/satchel to pass eslint checks.
https://reviewboard.mozilla.org/r/27093/#review24519
|
|
Assignee | |
Comment 23•9 years ago
|
||
Comment on attachment 8695376 [details]
MozReview Request: Bug 1229519: Fix toolkit/modules to pass eslint checks.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27083/diff/1-2/
Attachment #8695376 -
Flags: review?(felipc) → review?(mak77)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8695379 -
Flags: review?(gijskruitbosch+bugs) → review?(mak77)
|
|
Assignee | |
Comment 24•9 years ago
|
||
Comment on attachment 8695379 [details]
MozReview Request: Bug 1229519: Fix download managers to pass eslint checks.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27089/diff/1-2/
|
|
||
Comment 25•9 years ago
|
||
Comment on attachment 8695376 [details]
MozReview Request: Bug 1229519: Fix toolkit/modules to pass eslint checks.
https://reviewboard.mozilla.org/r/27083/#review24523
::: toolkit/modules/Promise-backend.js:239
(Diff revision 1)
> - let keys = [key for (key of this._map.keys())];
> + for (let key of [...this._map.keys()]) {
Array.from(this._map.keys())
::: toolkit/modules/RemotePageManager.jsm:468
(Diff revision 1)
> - messageManager.sendAsyncMessage("RemotePage:Register", { urls: [u for (u of this.pages.keys())] })
> + messageManager.sendAsyncMessage("RemotePage:Register", { urls: [...this.pages.keys()] })
Array.from
::: toolkit/modules/ZipUtils.jsm:86
(Diff revision 1)
> - readFailed(e);
> + readFailed(e);
please brace since part of an if-else
::: toolkit/modules/ZipUtils.jsm:118
(Diff revision 1)
> - return Task.spawn(function() {
> + return Task.spawn(function*() {
missing space after *
Attachment #8695376 -
Flags: review?(mak77) → review+
|
|
||
Comment 26•9 years ago
|
||
Comment on attachment 8695379 [details]
MozReview Request: Bug 1229519: Fix download managers to pass eslint checks.
https://reviewboard.mozilla.org/r/27089/#review24525
::: toolkit/components/downloads/test/unit/test_app_rep_maclinux.js:213
(Diff revision 1)
> -add_task(function()
> +add_task(function*()
missing space after *
::: toolkit/components/downloads/test/unit/test_app_rep_windows.js:313
(Diff revision 1)
> -add_task(function()
> +add_task(function*()
space after *
::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1716
(Diff revision 1)
> - ex.result == Cr.NS_ERROR_NOT_AVAILABLE) {
> + if (!(ex instanceof Components.Exception) || ex.result != Cr.NS_ERROR_NOT_AVAILABLE) {
could indent as
if (cond1 ||
cond2) {
::: toolkit/components/jsdownloads/src/DownloadList.jsm:236
(Diff revision 1)
> - Task.spawn(function() {
> + Task.spawn(function*() {
space after *
::: toolkit/components/jsdownloads/test/unit/head.js:535
(Diff revision 1)
> - return Task.spawn(function() {
> + return Task.spawn(function*() {
space after *
::: toolkit/mozapps/downloads/content/downloads.js:489
(Diff revision 1)
> -#ifndef XP_MACOSX
> + if (AppConstants.platform == "macosx" &&
This was an ifndef, so it should be != "macosx"
::: toolkit/mozapps/downloads/nsHelperAppDlg.js:1009
(Diff revision 1)
> -#else
> + else
please brace
There's an actual mistake here, apart from minor things, due to an ifndef conversion.
Attachment #8695379 -
Flags: review?(mak77) → review+
|
||
Comment 27•9 years ago
|
||
Comment on attachment 8695377 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/thumbnails to pass eslint checks.
https://reviewboard.mozilla.org/r/27085/#review24539
Attachment #8695377 -
Flags: review?(rhelmer) → review+
|
|
Assignee | |
Comment 28•9 years ago
|
||
Comment on attachment 8695384 [details]
MozReview Request: Bug 1229519: Fix toolkit/content to pass eslint checks.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27097/diff/1-2/
Attachment #8695384 -
Flags: review?(felipc) → review?(MattN+bmo)
|
||
Comment 29•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Whiteboard: keep-open
|
|
||
Comment 30•9 years ago
|
||
|
|
||
Comment 31•9 years ago
|
||
Comment on attachment 8695384 [details]
MozReview Request: Bug 1229519: Fix toolkit/content to pass eslint checks.
https://reviewboard.mozilla.org/r/27097/#review24547
r=me though I'm disappointed that try didn't catch the negation issue…
::: toolkit/content/globalOverlay.js:8
(Diff revision 1)
> -#ifndef XP_MACOSX
> + if (AppConstants.platform == "macosx") {
`#ifndef XP_MACOSX` should translate to `if (AppConstants.platform != "macosx") {`
::: toolkit/content/globalOverlay.js:28
(Diff revision 1)
> + }
> + else if (typeof(aPromptFunction) == "function" && !aPromptFunction()) {
y u no cuddle?
Attachment #8695384 -
Flags: review?(MattN+bmo) → review+
|
|
Assignee | |
Comment 32•9 years ago
|
||
https://reviewboard.mozilla.org/r/27097/#review24547
> `#ifndef XP_MACOSX` should translate to `if (AppConstants.platform != "macosx") {`
I doubt we have tests that verify the app shuts down when the last window is closed :(
|
|
||
Comment 33•9 years ago
|
||
Perhaps a test to see that the App doesn't shut down on OS X with no window would have also caught this? I'd have to dig in further to know for sure.
|
|
||
Comment 34•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Whiteboard: keep-open
|
||
Comment 35•9 years ago
|
||
> getCharsetInfo: function(charsets, sort=true) {
> - let list = [{
> + let list = Array.from(charsets, charset => ({
> label: this._getCharsetLabel(charset),
> accesskey: this._getCharsetAccessKey(charset),
> name: "charset",
> value: charset
> - } for (charset of charsets)];
> + }));
Nit: charsets is already an array, so charsets.map() would have worked.
|
||
Comment 36•9 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/312c5fa54cb9
https://hg.mozilla.org/mozilla-central/rev/acb7ba1c7eb9
https://hg.mozilla.org/mozilla-central/rev/d05e74081475
https://hg.mozilla.org/mozilla-central/rev/70431746380e
https://hg.mozilla.org/mozilla-central/rev/ad36ba7bbf39
https://hg.mozilla.org/mozilla-central/rev/43076a707aeb
https://hg.mozilla.org/mozilla-central/rev/6ad057f81520
https://hg.mozilla.org/mozilla-central/rev/f48ca84f3c1e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
|
||
Comment 37•9 years ago
|
||
The misc toolkit portion (5161ded671e0, not yet merged) was backed out in https://hg.mozilla.org/integration/fx-team/rev/f32a93d9abd5 (also not yet merged), so I'm reopening this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla45 → ---
|
|
Assignee | |
Comment 40•9 years ago
|
||
Relanded the bad changeset without the changes to toolkit/identity that seemed to upset Mulet tests. Added that file to .eslintignore for now.
|
||
Comment 41•9 years ago
|
||
| bugherder | ||
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
|
||
Comment 42•9 years ago
|
||
globalOverlay.js is used in some windows where AppConstants isn't already defined, so it should be imported into scope to be sure.
Attachment #8696797 -
Flags: review?(MattN+bmo)
|
|
||
Comment 43•9 years ago
|
||
Comment on attachment 8696797 [details] [diff] [review]
Followup to make sure AppConstants is defined in globalOverlay.js
Review of attachment 8696797 [details] [diff] [review]:
-----------------------------------------------------------------
Ah sorry, I didn't see the followup bug which already does the same thing.
Attachment #8696797 -
Flags: review?(MattN+bmo)
|
|
||
Updated•9 years ago
|
Attachment #8696797 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•